-
Notifications
You must be signed in to change notification settings - Fork 520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[orch] change Consumer class to support multiple values for the same key #1184
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
reload Consumer addToSync() and refactor ut code to use it Add google ut for consumer class Signed-off-by: Zhenggen Xu <[email protected]>
Signed-off-by: Zhenggen Xu <[email protected]>
Signed-off-by: Zhenggen Xu <[email protected]>
qiluo-msft
reviewed
Jan 29, 2020
qiluo-msft
reviewed
Jan 29, 2020
qiluo-msft
reviewed
Jan 29, 2020
qiluo-msft
reviewed
Jan 29, 2020
qiluo-msft
reviewed
Jan 29, 2020
qiluo-msft
reviewed
Jan 29, 2020
qiluo-msft
requested changes
Jan 29, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also change the title as a bug fix statement?
i am ok with this pr. |
lguohan
previously approved these changes
Jan 29, 2020
Signed-off-by: Zhenggen Xu <[email protected]>
zhenggen-xu
changed the title
[orch] Change the m_toSync in Consumer class to support both DEL and SET
[orch] change Consumer class to support multiple values for the same key
Jan 29, 2020
@qiluo-msft Feedback was addressed. |
qiluo-msft
approved these changes
Jan 29, 2020
lguohan
pushed a commit
that referenced
this pull request
Jan 30, 2020
…key (#1184) Description The Consumer class is used by the orch objects to deal with the redis consumer tables' popped items. It has m_toSync map to save the tasks. During the operations (more items than the orch objects can handle), the tasks in the map is merged for optimization. However, since it is a map, we would only have one value for each key. This potentially eliminate the necessary actions from Redis, e,g, we have a DEL action and SET action coming out from Redis for some key, we would overwrite the DEL action today in the code and thus not able to delete some objects as we intended. The PR changed the m_toSync from map to multi-map to get multiple values for one key. In this design, we keep maximun two values per key, DEL or SET or DEL+SET. We need strictly keep the order of DEL and SET. It is possible to use map of vectors to fulfill this, we chose multi-map because: 1, It will have less/no changes to different orch classes to iterate the m_toSync 2, The order can be guaranteed. The order of the key-value pairs whose keys compare equivalent is the order of insertion and does not change. (since C++11). See https://en.cppreference.com/w/cpp/container/multimap The PR also refactors the consumer class so vlanmgr.cpp and routeorch.cpp will leverage the Consumer functions instead of operating on the members. It also refactors the UT code (aclorch_ut.cpp) so it removes the redundant code and uses the same code. Google UT tests were added for Consumer Class especially for different cases for addToSync() function. What I did Change the m_toSync in Consumer class to multimap so it could support both DEL and SET Reload Consumer addToSync() and refactor vlanmgr/route-orch and ut code to use it Add google ut for consumer class Why I did it See description. How I verified it Unit tests: Running main() from gtest_main.cc ``` [==========] Running 19 tests from 5 test cases. [----------] Global test environment set-up. [----------] 1 test from AclTest [ RUN ] AclTest.Create_L3_Acl_Table [ OK ] AclTest.Create_L3_Acl_Table (1 ms) [----------] 1 test from AclTest (1 ms total) [----------] 3 tests from AclOrchTest [ RUN ] AclOrchTest.ACL_Creation_and_Destorying [ OK ] AclOrchTest.ACL_Creation_and_Destorying (1000 ms) [ RUN ] AclOrchTest.L3Acl_Matches_Actions [ OK ] AclOrchTest.L3Acl_Matches_Actions (1001 ms) [ RUN ] AclOrchTest.L3V6Acl_Matches_Actions [ OK ] AclOrchTest.L3V6Acl_Matches_Actions (1000 ms) [----------] 3 tests from AclOrchTest (3003 ms total) [----------] 2 tests from PortsOrchTest [ RUN ] PortsOrchTest.PortReadinessColdBoot [ OK ] PortsOrchTest.PortReadinessColdBoot (21 ms) [ RUN ] PortsOrchTest.PortReadinessWarmBoot [ OK ] PortsOrchTest.PortReadinessWarmBoot (13 ms) [----------] 2 tests from PortsOrchTest (34 ms total) [----------] 4 tests from SaiSpy [ RUN ] SaiSpy.CURD [ OK ] SaiSpy.CURD (0 ms) [ RUN ] SaiSpy.Same_Function_Signature_In_Same_API_Table [ OK ] SaiSpy.Same_Function_Signature_In_Same_API_Table (0 ms) [ RUN ] SaiSpy.Same_Function_Signature_In_Different_API_Table [ OK ] SaiSpy.Same_Function_Signature_In_Different_API_Table (0 ms) [ RUN ] SaiSpy.create_switch_and_acl_table [ OK ] SaiSpy.create_switch_and_acl_table (0 ms) [----------] 4 tests from SaiSpy (0 ms total) [----------] 9 tests from ConsumerTest [ RUN ] ConsumerTest.ConsumerAddToSync_Set [ OK ] ConsumerTest.ConsumerAddToSync_Set (1 ms) [ RUN ] ConsumerTest.ConsumerAddToSync_Del [ OK ] ConsumerTest.ConsumerAddToSync_Del (0 ms) [ RUN ] ConsumerTest.ConsumerAddToSync_Set_Del [ OK ] ConsumerTest.ConsumerAddToSync_Set_Del (0 ms) [ RUN ] ConsumerTest.ConsumerAddToSync_Del_Set [ OK ] ConsumerTest.ConsumerAddToSync_Del_Set (130 ms) [ RUN ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi [ OK ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi (204 ms) [ RUN ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi_In_Q [ OK ] ConsumerTest.ConsumerAddToSync_Set_Del_Set_Multi_In_Q (4 ms) [ RUN ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew [ OK ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew (0 ms) [ RUN ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew1 [ OK ] ConsumerTest.ConsumerAddToSync_Del_Set_Setnew1 (0 ms) [ RUN ] ConsumerTest.ConsumerAddToSync_Ind_Set_Del [ OK ] ConsumerTest.ConsumerAddToSync_Ind_Set_Del (0 ms) [----------] 9 tests from ConsumerTest (340 ms total) [----------] Global test environment tear-down [==========] 19 tests from 5 test cases ran. (4344 ms total) [ PASSED ] 19 tests. ``` Signed-off-by: Zhenggen Xu <[email protected]>
oleksandrivantsiv
pushed a commit
to oleksandrivantsiv/sonic-swss
that referenced
this pull request
Mar 1, 2023
* c0bdac2 2021-01-24 | [saithrift]: add correct libs to build saithrift on libsaivs (sonic-net#1184) (HEAD, origin/v1.7) [lguohan] Signed-off-by: Guohan Lu <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The Consumer class is used by the orch objects to deal with the redis consumer tables' popped items. It has m_toSync map to save the tasks. During the operations (more items than the orch objects can handle), the tasks in the map is merged for optimization. However, since it is a map, we would only have one value for each key. This potentially eliminate the necessary actions from Redis, e,g, we have a DEL action and SET action coming out from Redis for some key, we would overwrite the DEL action today in the code and thus not able to delete some objects as we intended.
The PR changed the m_toSync from map to multi-map to get multiple values for one key. In this design, we keep maximun two values per key, DEL or SET or DEL+SET. We need strictly keep the order of DEL and SET. It is possible to use map of vectors to fulfill this, we chose multi-map because:
1, It will have less/no changes to different orch classes to iterate the m_toSync
2, The order can be guaranteed. The order of the key-value pairs whose keys compare equivalent is the order of insertion and does not change. (since C++11). See
https://en.cppreference.com/w/cpp/container/multimap
The PR also refactors the consumer class so vlanmgr.cpp and routeorch.cpp will leverage the Consumer functions instead of operating on the members. It also refactors the UT code (aclorch_ut.cpp) so it removes the redundant code and uses the same code.
Google UT tests were added for Consumer Class especially for different cases for addToSync() function.
What I did
Change the m_toSync in Consumer class to multimap so it could support both DEL and SET
Reload Consumer addToSync() and refactor vlanmgr/route-orch and ut code to use it
Add google ut for consumer class
Why I did it
See description.
How I verified it
Unit tests:
Running main() from gtest_main.cc
Details if related